Repository navigation
Conversation
…op folding non-ascii strings to NaN
ExprData::to_number returned None for any string with a rope segment, so
+("1" + "2") was printed as +"12" under minify syntax (and in enum
bodies), and a second transpile of the output folded it to 12. It now
flattens the rope through the arena first, and the inlined-enum arm
recurses into the inlined value instead of duplicating the string arm.
string_to_equivalent_number_value folded every non-ascii utf-8 string to
NaN; U+00A0 and other non-ascii whitespace are stripped by StringToNumber,
so a --define value like "\u00a01" folded to the wrong constant. It now
declines, leaving the expression unfolded like utf-16 literals already are.
|
Status: fix and tests pushed, self-review in progress. Reproduced on bun 1.4.0 with Fix is in this PR (#39072): |
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit 20895ec has some failures in 🧪 To try this PR locally: bunx bun-pr 39072That installs a local version of the PR into your bun-39072 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughChangesUnary numeric constant folding now resolves rope strings and inlined enum values through arena-aware conversion. Unsupported and non-ASCII strings remain unfurled. Bundler and transpiler tests cover unary operators, whitespace, enums, defined values, and dynamic operands. Unary constant folding
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized change fixes incorrect constant folding for string ropes and non-ASCII defined values, with regression coverage and no actionable merge-blocking risk remaining beyond normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — targeted constant-folding fix with a clear mechanism and thorough tests.
Checked that the let mut str = *str; str.resolve_rope_if_needed(bump) pattern matches the existing StoreRef copy-then-DerefMut idiom used at six other sites in expr.rs, and that EString::push debug-asserts both segments are UTF-8, so resolve_rope_if_needed's root-only is_utf8() gate is sound for ropes.
Confirmed the only ExprData::to_number callers are the three SideEffects::to_number sites in e_unary (all updated) plus the new EInlinedEnum recursion; the other to_number(global) hits are JSValue::to_number.
Verified string_to_equivalent_number_value returning None on non-ascii only narrows folding (leaves the expression as-is), never miscomputes.
Extended reasoning...
Overview
The PR fixes two constant-folding issues in ExprData::to_number (src/ast/expr.rs): (1) it now flattens rope strings via resolve_rope_if_needed(bump) before converting, so +("1" + "2") folds to 12 in one pass instead of +"12" then 12; (2) string_to_equivalent_number_value now returns Option<f64> and declines non-ascii UTF-8 strings instead of folding them to NaN (which was wrong for e.g. U+00A0-prefixed numbers from --define). The EInlinedEnum arm is deduplicated into a recursive call. SideEffects::to_number and its three call sites in visit_expr.rs are updated to thread p.arena through. Tests are added to transpiler.test.js (rope folds, fixed-point, enum, define) and bundler_minify.test.ts (two itBundled cases through bun build).
Security risks
None. This is compile-time constant folding of string/number literals in the transpiler. The change only narrows incorrect folding (non-ascii → decline) and extends correct folding (ropes → flatten first). Declining to fold is always safe — the expression is left as-is and evaluated at runtime. No untrusted input reaches a new code path; JSC__jsToNumber was already called on the same bytes.
Level of scrutiny
Medium. Constant folding bugs can silently corrupt bundler output, so correctness matters. But the change is small (~30 lines of logic), the mechanism is fully explained in the PR description, and the fix follows established patterns: the StoreRef copy + DerefMut in-place mutation is used identically at expr.rs:872-876 and expr.rs:2799-2820. I verified EString::push debug-asserts all rope segments are UTF-8, so resolve_rope_if_needed's root-only encoding check is sound and slice8() after flattening reads the whole string. I grepped for all to_number and SideEffects::to_number callers — the three e_unary sites are the only ones on ExprData (the rest are JSValue::to_number(global)).
Other factors
Test coverage is strong: fixed-point assertions (transform(transform(x)) === transform(x)), enum-without-minify cases, --define UTF-8 cases with both ascii (folds) and non-ascii (declines), negative cases (+("1" + y) stays unfolded), and end-to-end via itBundled. The author reports all five new tests fail on 1.4.0 and pass with the fix, and ran the full transpiler/bundler/esbuild suites plus clippy. The EInlinedEnum recursion technically now also matches ENull/EUndefined/EBoolean, but per the EnumNumber/EnumString construction those never occur, and even if they did the fold would be correct. No prior reviews to address.
|
#46915 has the non-ASCII half of this PR: |
|
#47048 adds With it, each arm can flatten the rope as this PR does, and then return
The branch
What was checked. On the current base the commit compiles (
A review of that commit asked for rows that it does not have yet:
|
Problem
minify: { syntax: true },x = +("1" + "2")is printed asx = +"12"; transpiling that output again givesx = 12. Same for-("1" + "2")(-"12", then-12) and~("1" + "2")(~"12", then-13). Atransform(transform(x)) === transform(x)check flags it.enum E { A = "1" + "2", B = +("3" + "4"), C = -A }emitsE[E["B"] = +"34"] = "B"andE[E["C"] = -"12"] = "C", soBandCare not numeric constants and their uses are not inlined. esbuild emits34and-12."1" + "2"is folded into a rope (see Background), andExprData::to_number(src/ast/expr.rs) returnedNoneas soon as the string had anextsegment, in both itsEStringarm and itsEInlinedEnumarm. The+/-folds and the~fold ine_unary(src/js_parser/visit/visit_expr.rs) therefore skipped ropes; the printer flattens the rope, so the second pass saw a plain literal and folded it.string_to_equivalent_number_valuefolded every UTF-8 string containing non-ascii bytes toNaN. Non-ascii source literals are stored as UTF-16 and were never folded, but the strings bun injects as UTF-8 (--definevalues, inlinedprocess.env) are: with--define 'X="\u00a01"'(U+00A0 is whitespace toStringToNumber, so the value is1),+XbecameNaNand, under minify,~Xbecame-1(the JS values are1and-2). Flattening ropes on its own would have extended this to+(X + "1"), which is left unfolded today.Fix
to_numbertakes the arena and flattens a rope before converting it;str.resolve_rope_if_needed(bump)is the fixing line. TheEInlinedEnumarm recurses into the inlined value instead of carrying a copy of the string arm; an inlined enum value is only ever anENumberor a copy of the member'sEString(theEnumNumber/EnumStringarms inp.rsandfold.rs), so the arm folds what it folded before, plus ropes.string_to_equivalent_number_valuereturnsNonefor a non-ascii string instead ofNaN;return Noneis the fixing line.JSC__jsToNumberreads latin1, so only ascii bytes can be handed to it as-is; declining leaves the expression unfolded, which is what UTF-16 literals already get.data/nextonly and does not touch the linked segments, which is the in-place flatteningData::eqlandextract_string_valuesin the same file already perform on these nodes, inlined enum copies included.+("1" + "2")and the enum members above fold there.SideEffects::to_number(scan_side_effects.rs) takes the arena too and its three uses ine_unarypassp.arena; there are no other callers. (js_parser: fold the unary produced by distributing it over a comma operand #39049 reshuffles the same three lines ine_unary; whichever lands second has a one-line conflict per call.)test/bundler/transpiler/transpiler.test.js("simplification": the rope folds and their fixed point, the enum cases, thedefinecases) andtest/bundler/bundler_minify.test.ts(minify/ConstantFoldingUnaryOnFoldedString,minify/ConstantFoldingUnaryPlusNonAsciiDefine, throughbun build). All five fail on bun 1.4.0 (+"12",E[E["B"] = +"34"],x = NaN,E.B + E.C + E.Dleft as is) and pass with this change.transpiler.test.js,bundler_minify.test.ts,bundler_string.test.ts,bundler_edgecase.test.ts,bundler_regressions.test.ts,transpiler_constant_fold_eqeq.test.ts,esbuild/ts.test.ts,esbuild/dce.test.ts,esbuild/default.test.ts,transpiler/*; andcargo clippy -p bun_ast -p bun_js_parser.Background
E::Stringnode (next/end/rope_leninsrc/ast/e.rs,src/ast/fold_string_addition.rs) and the printer walks the chain.resolve_rope_if_neededreplaces the root node's bytes with the concatenation (allocated in the parser's arena, hence the new parameter) and clearsnext. Anything that reads a folded string's bytes has to flatten first or walk the chain;dataalone is the first piece.E::InlinedEnumwrapping a copy of the member's number or string (printed as12 /* A */). Enum initializers are folded whatever the minify settings, so a member defined by concatenation is a rope and each use of it is a rope.should_fold_typescript_constant_expressions: the parser flag under which string addition is folded (so ropes exist at all) and under which~folds. It is on under minify syntax and always on inside enum bodies. The+x/-xfolds run unconditionally, which is why the enum case reproduces without minify.